Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add offline message to IOU and Split bill and allow currency selection while offline #4019

Merged
merged 14 commits into from
Jul 22, 2021
Merged

Add offline message to IOU and Split bill and allow currency selection while offline #4019

merged 14 commits into from
Jul 22, 2021

Conversation

luacmartins
Copy link
Contributor

@luacmartins luacmartins commented Jul 13, 2021 •
edited
Loading

cc @chiragsalian
cc @tgolen and @marcaaron because of changes to processNetworkRequestQueue

Currently, requests that are sent offline and should not be retried are hanging since the promises never resolve. This PR should solve that and allow users to go through the currency selection, as well as see an offline error message on the confirmation page.

Fixed Issues

$ #3792

Tests

  1. Login to app
  2. Kill internet connection
  3. Click '+' and select Request Money
  4. Verify that you can go through the flow and see Looks like you're offline. Please check your connection and try again. error message on the request confirmation page.
  5. Dismiss the modal
  6. Click '+' and select Split Bill
  7. Verify that you can go through the flow and see Looks like you're offline. Please check your connection and try again. error message on the request confirmation page.

QA Steps

Steps above.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

web.mov

Mobile Web

mobile-web.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

@luacmartins luacmartins self-assigned this Jul 13, 2021
@luacmartins luacmartins force-pushed the cmartins-offline-currency branch from 63d5d7b to de88d27 Compare July 14, 2021 21:57
@luacmartins luacmartins requested a review from a team July 14, 2021 22:23
@luacmartins luacmartins marked this pull request as ready for review July 14, 2021 22:23
@MelvinBot MelvinBot requested review from MariaHCD and removed request for a team July 14, 2021 22:23
marcaaron
marcaaron requested changes Jul 15, 2021
View reviewed changes
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, it seems like this PR is solving two separate issues?

  1. Disallow creating IOUs while offline
  2. Preventing requests to get a preferred currency while offline from "hanging" because promises that should not be retried never resolve.

The code changes for 1 look straightforward. I'd like to discuss the changes for 2 some more also wondering whether this can be solved by adding some kind of default currency as suggested here

src/libs/Network.js Outdated Show resolved Hide resolved
src/libs/Network.js Outdated Show resolved Hide resolved
src/libs/actions/PersonalDetails.js Outdated Show resolved Hide resolved
src/libs/Network.js Outdated Show resolved Hide resolved
@luacmartins
Copy link
Contributor Author

luacmartins commented Jul 15, 2021 •
edited
Loading

Hey @marcaaron! Thank you so much for the awesome feedback - I really appreciate it!

Just to clarify, it seems like this PR is solving two separate issues?

Correct! The goal for 2 is to allow users to go through the IOU flow and get to the confirmation page while offline. We currently set a default currency, but the issue is that we never set isRetrievingCurrency back to false when we call fetchCurrencyPreferences

@luacmartins luacmartins force-pushed the cmartins-offline-currency branch from de88d27 to 8f10ad7 Compare July 16, 2021 18:31
@luacmartins luacmartins requested a review from a team as a code owner July 16, 2021 18:31
@MelvinBot MelvinBot requested review from yuwenmemon and removed request for a team July 16, 2021 18:32
@luacmartins
Copy link
Contributor Author

Updated! Changes to Network were reverted and the solution now implements a second selectedCurrency key.

@luacmartins luacmartins removed the request for review from yuwenmemon July 16, 2021 18:40
marcaaron
marcaaron requested changes Jul 16, 2021
View reviewed changes
src/pages/iou/IOUCurrencySelection.js Outdated Show resolved Hide resolved
src/pages/iou/IOUModal.js Show resolved Hide resolved
src/pages/iou/IOUModal.js Outdated Show resolved Hide resolved
Julesssss
Julesssss reviewed Jul 19, 2021
View reviewed changes
src/libs/actions/IOU.js Outdated Show resolved Hide resolved
Julesssss
Julesssss reviewed Jul 19, 2021
View reviewed changes
src/libs/actions/IOU.js Outdated Show resolved Hide resolved
@luacmartins
Copy link
Contributor Author

Updated with requested changes! I will create a PR in Web-E renaming preferredCurrency to localCurrency in the API.

@luacmartins luacmartins changed the title Add offline message to IOU and Split bill and allow currency selection while offline [HOLD Web-E PR #31503] Add offline message to IOU and Split bill and allow currency selection while offline Jul 19, 2021
marcaaron
marcaaron requested changes Jul 19, 2021
View reviewed changes
src/libs/actions/IOU.js Show resolved Hide resolved
src/libs/actions/IOU.js Outdated Show resolved Hide resolved
src/libs/actions/PersonalDetails.js Outdated Show resolved Hide resolved
src/pages/iou/IOUCurrencySelection.js Outdated Show resolved Hide resolved
@luacmartins
Copy link
Contributor Author

Updated!

Julesssss
Julesssss previously approved these changes Jul 22, 2021
View reviewed changes
@Julesssss
Copy link
Contributor

Leaving this to @marcaaron, as he also requested changes

marcaaron
marcaaron previously approved these changes Jul 22, 2021
View reviewed changes
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left some small comments

src/pages/iou/steps/IOUAmountPage.js Outdated Show resolved Hide resolved
src/pages/iou/steps/IOUAmountPage.js Outdated Show resolved Hide resolved
@luacmartins luacmartins dismissed stale reviews from marcaaron and Julesssss via 180bef2 July 22, 2021 16:53
@luacmartins
Copy link
Contributor Author

Updated!

@marcaaron marcaaron merged commit fd92e19 into main Jul 22, 2021
@marcaaron marcaaron deleted the cmartins-offline-currency branch July 22, 2021 17:31
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions bot pushed a commit that referenced this pull request Jul 23, 2021
@marcaaron
Merge pull request #4019 from Expensify/cmartins-offline-currency
91f797b 
Add offline message to IOU and Split bill and allow currency selection while offline

(cherry picked from commit fd92e19)
@OSBotify OSBotify mentioned this pull request Jul 23, 2021
Merged
@Julesssss
Copy link
Contributor

Julesssss commented Jul 23, 2021 •
edited
Loading

I requested a CP for this PR as deploying it will fix this issue sooner. Until then, some users will not be able to make IOU requests.

Update: I just realized this wasn't necessary as we already had a PR on staging which made the API change 🤦

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.79-4🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify OSBotify mentioned this pull request Jul 23, 2021
Closed
60 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-4🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.79-5🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.80-2🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

This was referenced Jul 28, 2021
Closed
Closed
This was referenced Aug 6, 2021
Closed
Closed
@OSBotify OSBotify mentioned this pull request Aug 17, 2021
Closed
This was referenced Aug 25, 2021
Closed
Closed
This was referenced Sep 2, 2021
Closed
Closed
@OSBotify OSBotify mentioned this pull request Aug 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcaaron marcaaron marcaaron approved these changes

@Julesssss Julesssss Julesssss left review comments

Assignees

@luacmartins luacmartins

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@luacmartins @Julesssss @OSBotify @marcaaron